Skip to content

Add missing networking_pods dependencies to Yosemite in Podfile #8418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 15, 2022
Merged

Add missing networking_pods dependencies to Yosemite in Podfile #8418

merged 1 commit into from
Dec 15, 2022

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Dec 15, 2022 •
edited
Loading

This fixes the CI failure experienced in #8400.

The reason those failures occurred is, to my best understanding, that Yosemite depends on Networking (e.g. in Store.swift) but the dependency was never explicit in the Podfile. I think this was never an issue becausenetworking_pods and yosemite_pods mostly overlapped, namely, all the CocoaPods from networking_pods are also specified in yosemite_pods. In trunk:

woocommerce-ios/Podfile

Lines 171 to 181 in c2bb7fe

def networking_pods
alamofire
cocoa_lumberjack
pod 'Sourcery', '~> 1.0.3', configuration: 'Debug'
# Used for HTML parsing
aztec
wordpress_kit
end

woocommerce-ios/Podfile

Lines 120 to 127 in c2bb7fe

def yosemite_pods
alamofire
stripe_terminal
cocoa_lumberjack
wordpress_kit
aztec
end

But, when #8400 added KeychainAccess to networking_pods it introduced a difference that broke the tests at runtime, resulting in the error seen in CI:

The bundle “YosemiteTests� couldn’t be loaded.
The bundle couldn’t be loaded.
Try reinstalling the bundle.
dlopen(/usr/local/var/buildkite-agent/builds/MV-MKE-X64-008/automattic/woocommerce-ios/DerivedData/Build/Products/Debug-iphonesimulator/YosemiteTests.xctest/YosemiteTests, 0x0109):
  Library not loaded:
    @rpath/KeychainAccess.framework/KeychainAccess

By explicitly adding networking_pods as a dependency in yosemite_pods, the issue is fixed because all the required frameworks are now available.

It's worth noting that only the checksum changed in Podfile.lock because this change doesn't add any new dependency to the project itself. The only difference would be in how Pods.xcodeproj is generated, but that is a file we don't track in Git.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 15, 2022 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8418-7b5dbe0 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@mokagio mokagio force-pushed the mokagio/test-8394-ci-fix branch from c293ce2 to 7b5dbe0 Compare December 15, 2022 11:02
@mokagio mokagio changed the title Add missing KeychainAccess requirement to Yosemite in Podfile Add missing networking_pods dependencies to Yosemite in Podfile Dec 15, 2022
mokagio
mokagio commented Dec 15, 2022
View reviewed changes
Podfile
stripe_terminal
cocoa_lumberjack
wordpress_kit
networking_pods
Copy link
Contributor Author

@mokagio mokagio Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR description I wrote "Yosemite depends on Networking (e.g. in Store.swift) but the dependency was never explicit in the Podfile".

This change doesn't make the dependency explicit either, to be fair, but is close enough. On the top of my mind, I'm not sure whether we can make one target depend on another with our multiple projects setup. I don't think it's worth spending additional time on this right now because we may be migrating away from CocoaPods soon-ish. So, as long as the app builds, we're good.

Copy link
Contributor

@AliSoftware AliSoftware Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I think it would be possible to do it by using abstract_target. But I agree it's not worth spending the time.

@mokagio mokagio marked this pull request as ready for review December 15, 2022 11:28
@mokagio mokagio self-assigned this Dec 15, 2022
@mokagio mokagio added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label Dec 15, 2022
@mokagio mokagio added this to the 11.7 milestone Dec 15, 2022
@mokagio mokagio mentioned this pull request Dec 15, 2022
Merged
1 task
@mokagio mokagio requested a review from itsmeichigo December 15, 2022 11:30
selanthiraiyan
selanthiraiyan approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@selanthiraiyan selanthiraiyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining the changes in detail. 🎉

@selanthiraiyan selanthiraiyan merged commit cd306a2 into feat/8394-generate-application-password Dec 15, 2022
@selanthiraiyan selanthiraiyan deleted the mokagio/test-8394-ci-fix branch December 15, 2022 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AliSoftware AliSoftware AliSoftware left review comments

@itsmeichigo itsmeichigo Awaiting requested review from itsmeichigo

+1 more reviewer

@selanthiraiyan selanthiraiyan selanthiraiyan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mokagio mokagio

Labels

category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc.

Projects

None yet

Milestone

11.7

Development

Successfully merging this pull request may close these issues.

5 participants

@mokagio @wpmobilebot @AliSoftware @selanthiraiyan